-
Notifications
You must be signed in to change notification settings - Fork 798
[DevSAN] Change DeviceType to specialization constant #19798
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[DevSAN] Change DeviceType to specialization constant #19798
Conversation
This will allow backend compiler to eliminte unnecessary code to reduce kernel code size.
unified-runtime/source/loader/layers/sanitizer/sanitizer_common/sanitizer_libdevice.hpp
Outdated
Show resolved
Hide resolved
Hi @steffenlarsen, I have another question. The pre-ci for testing ABI compatibility is failed due to this PR will indeed break ABI compatibility. However, the ABI compatibility is unimportant to device sanitizer since the user must re-build their code if they want to test their code with device sanitizer. Do you know if there is a way to disable a test under ABI compatibility testing mode? |
I believe it should be as simple as adding the tests at the top of devops/compat_ci_exclude.sycl-rel-6_3 with a comment stating why it is OK for it to break compatibility. |
I'm not sure @xtian-github agrees with that: #19719 (comment) |
Hi @xtian-github, could you please help review changes in 'devops/compat_ci_exclude.sycl-rel-6_3'. I disabled sanitizer tests since backward compatibility is unimportant for device sanitizer. |
Kindly ping @gmlueck, @xtian-github |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM.
A question: BMG is not a supported target? What is the engineering cost to enable it? Can the DG2 path be leveraged?
BMG is not a supported target. We do not validate device-side sanitizer on BMG before so we don't know the gap to support it. |
Hi @intel/llvm-gatekeepers, there is a pre-ci failure with hung test. And the hung issue is unrelated to this change. I add comments to issue #19786 to track this problem. Could you please help merge this PR? |
Hi @intel/llvm-gatekeepers, this PR is ready to be merged, thanks. |
This will allow backend compiler to eliminte unnecessary code to reduce kernel code size.